fix: shield coverage result polling #6847

lwin-kyaw · 2025-10-15T21:46:11Z

Explanation

This PR includes ~

Updated internal coverage result polling and log logic.
- Added cancellation logic to the polling.
- Updated implementation of timeout.
- Cancel any pending requests before starting new polling or logging.
Updated TransactionMeta comparison in TransactionController:stateChange subscriber to avoid triggering multiple check coverage result unnecessarily.
Removed Personal Sign check from the check signature coverage result.

References

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Note

Implements cancellable/timeout-aware coverage result polling, cancels pending requests before new polls/logs, refines tx simulation change detection, and removes the personal_sign-only guard for signature coverage; updates tests and changelog.

Backend:
- Introduces PollingWithTimeoutAndAbort for coverage result polling with timeout, abort, and per-request IDs.
- Reworks #getCoverageResult to use abort signals; throws on non-200; updates timeout error message.
- Cancels pending polls before logSignature/logTransaction.
Controller:
- Removes personal_sign restriction; check signature coverage for any new SignatureRequest.
- Adds #compareTransactionSimulationData and uses it to re-check coverage when simulation data meaningfully changes.
Tests:
- Adds unit tests for polling utility and simulation-data change scenarios; adjusts backend tests (timeout message, cleanup).
Docs:
- Updates CHANGELOG.md with polling/logging changes, refined tx comparison, and signature-check note.

^{Written by Cursor Bugbot for commit a5295ab. This will update automatically on new commits. Configure here.}

…olling.

lwin-kyaw · 2025-10-15T21:57:09Z

@metamaskbot publish-preview

…lean up

lwin-kyaw · 2025-10-16T04:53:45Z

@metamaskbot publish-preview

matthiasgeihs

Please note my comment on using a singular abort controller for unrelated result requests. I think it's highly problematic. You only want to cancel polling for that specific coverageId, not for all coverage requests.

matthiasgeihs · 2025-10-16T06:03:49Z

packages/shield-controller/src/ShieldController.ts

+      const simulationDataChanged = this.#compareTransactionSimulationData(
+        previousTransaction?.simulationData,
+        transaction.simulationData,
+      );


Bot must have had a bad day. That's not the case, as far as I can see.

matthiasgeihs · 2025-10-16T06:14:40Z

packages/shield-controller/src/backend.ts

      pollInterval?: number;
    },
  ): Promise<GetCoverageResultResponse> {
+    if (this.#abortController && !this.#abortController.signal.aborted) {


The way you integrated the abort functionality is problematic.

You use one abort controller for all requests.

There may be situations where you have result polling for different transactions and signatures at the same time. If one log request comes in, it cancels the polling from all other unrelated transactions and signatures. That's most definitely not what we want.

I have updated the design with Map<transactionId | signatureId, AbortController to handle multiple requests.
9225267

matthiasgeihs · 2025-10-16T07:02:00Z

packages/shield-controller/src/backend.ts

+    let shouldContinuePolling = true;
+    const poll = async (): Promise<GetCoverageResultResponse> => {
+      // Poll until the coverage result is ready or the abort signal is triggered.
+      while (shouldContinuePolling && !abortController.signal.aborted) {
+        const startTime = Date.now();
+        const res = await this.#fetch(configs.coverageResultUrl, {
+          method: 'POST',
+          headers,
+          body: JSON.stringify(reqBody),
+          signal: abortController.signal,
+        });
+        if (res.status === 200) {
+          shouldContinuePolling = false;
+          return (await res.json()) as GetCoverageResultResponse;
+        }
+        if (!abortController.signal.aborted) {
          await sleep(pollInterval - (Date.now() - startTime));
        }
-        // The following line will not have an effect as the upper level promise
-        // will already be rejected by now.
-        throw new Error('unexpected error');
-      };
+      }
+      // The following line will not have an effect as the upper level promise
+      // will already be rejected by now.
+      throw new Error('unexpected error');
+    };

-      poll().then(resolve).catch(reject);
-    });
+    return await withTimeoutAndCancellation<GetCoverageResultResponse>(
+      poll,
+      timeout,
+      abortController,
+    );


Instead of rewriting the entire logic, you could have just used the existing variable timeoutReached (maybe rename it) to cancel the polling.

We should avoid refactoring existing logic, if not necessary. I suspect the logic might have been unclear and that's why you have rewritten it. Instead you could also check with the author to clarify the existing code.

Restore the existing logic and appended the cancellation logic in this commit, 9225267

packages/shield-controller/src/backend.ts

…equests

matthiasgeihs · 2025-10-17T08:25:14Z

packages/shield-controller/src/backend.ts

+
+      const abortHandler = () => {
+        timeoutReached = true;
+        this.#removeAbortHandler(configs.requestId, abortHandler);
+        reject(new Error('Coverage result polling cancelled'));
+      };
+      abortController.signal.addEventListener('abort', abortHandler);
+
      setTimeout(() => {
        timeoutReached = true;
+        this.#removeAbortHandler(configs.requestId, abortHandler);
        reject(new Error('Timeout waiting for coverage result'));
      }, timeout);


We could remove some redundancy here. Would it be possible to have the cleanup and rejection in just one place? Maybe you could simplify call abortController.abort() when the timeout triggers?

timeoutReached = true; this.#removeAbortHandler(configs.requestId, abortHandler); reject(new Error(msg));

matthiasgeihs · 2025-10-17T08:26:39Z

packages/shield-controller/src/backend.ts

      setTimeout(() => {
        timeoutReached = true;
+        this.#removeAbortHandler(configs.requestId, abortHandler);
        reject(new Error('Timeout waiting for coverage result'));
      }, timeout);


Since you are already using an AbortSignal above, you could also use an AbortSignal.timeout here (instead of setTimeout).

Please advise me if I'm wrong.

One problem with using AbortSignal.timeout is that, we can't manually cancel the timeout.
Let's say when another request comes in, we need to cancel the pending requests right?
Cancelling request needs two things here

cancel the abort controller (to stop polling)

cancel the timeout

For the timeout cancel, if we are using JS timers, we can simply cancel the timers. However, there's no way to cancel the AbortSignal.timeout, which will eventually throw which would be problematic, too.

matthiasgeihs · 2025-10-17T08:28:03Z

packages/shield-controller/src/backend.ts

            method: 'POST',
            headers,
            body: JSON.stringify(reqBody),
+            signal: abortController.signal,


This does not respect the timeout.
You can have a look at my draft, how the two can be combined:

core/packages/shield-controller/src/backend.ts

Line 224 in 40ff4f5

timeoutSignal.onabort = () => abortController.abort();

Similarly, the while condition does not respect the abortController.signal.

Yes, this doesn't respect the timeout. But this cancel the ongoing network request, when the abortController is aborted (aka new request is triggered).

To be frank, this (AbortController with network request) is a very common pattern (especially in frontend/react).
Ref: https://stackoverflow.com/a/61056569

Similarly, the while condition does not respect the abortController.signal.

When abortController.signal is aborted, we set timeoutReached=true where break the while loop.

matthiasgeihs · 2025-10-17T08:30:35Z

packages/shield-controller/src/backend.ts

+    const abortController = this.#abortControllerMap.get(requestId);
+    if (abortController) {
+      if (abortHandler) {
+        abortController.signal.removeEventListener('abort', abortHandler);


I'm wondering: Is it necessary to remove the handler, if we drop the whole controller anyways? (Might simplify a few things.)

I think we should clean up (removeEventListener) whenever we do addEventListener.
Eventually, the event listeners will be clean up when the event target (AbortController) is garbage collected. But still memory leaks can if AbortController is not GC, due to some unknown reasons :)

matthiasgeihs · 2025-10-17T08:36:44Z

packages/shield-controller/tests/mocks/abortController.ts

I don't see any use of mocking the AbortController. On the downside, it bloats the codebase and can also cause some issues (e.g., it currently doesn't work with AbortSignal.any). We should remove this mock.

My bad on not removing the MockAbortController when it's no longer needed anymore.
I will take note of that, and will take a closer and more careful look in the future. 🙏

matthiasgeihs · 2025-10-17T08:50:15Z

packages/shield-controller/src/backend.test.ts

+      const coverageId = 'coverageId';
+
+      // Mock fetch responses
+      fetchMock.mockImplementation(


This mock implementation is in parts redundant to the other fetchMock implementations.

All these new tests were refactored and removed here, 565c42c

matthiasgeihs · 2025-10-17T08:51:15Z

packages/shield-controller/src/backend.test.ts

+      const signatureRequest = generateMockSignatureRequest();
+      const coverageId = 'test';
+
+      fetchMock.mockImplementation(


This mock implementation is in parts redundant to the other fetchMock implementations.

All these new tests were refactored and removed here, 565c42c

matthiasgeihs · 2025-10-17T08:57:27Z

packages/shield-controller/src/backend.ts

    };
  }
+
+  #cancelPreviousPendingRequests(id: string) {


Suggested change

#cancelPreviousPendingRequests(id: string) {

#cancelPendingRequests(id: string) {

If it's "pending" request, it's always a "previous" request. So we can omit previous.

All these logics were refactored and removed here, 565c42c

matthiasgeihs · 2025-10-17T09:04:50Z

packages/shield-controller/src/backend.ts

      coverageId,
    };

+    const abortController = this.#assignNewAbortController(configs.requestId);


Is there a reason why you create this here and not within the Promise?

(We could probably move all things from the outer context into the Promise. Might be cleaner overall.)

All these logics were refactored and removed here, 565c42c

matthiasgeihs · 2025-10-17T09:12:49Z

packages/shield-controller/src/backend.ts

+
+      const abortHandler = () => {
+        timeoutReached = true;
+        this.#removeAbortHandler(configs.requestId, abortHandler);


Unrelated to here, but could we change configs to config? Configuration is usually used in singular. Alternatively, you could use opts or options.

All these logics were refactored and removed here, 565c42c

lwin-kyaw · 2025-10-17T21:47:40Z

@metamaskbot publish-preview

matthiasgeihs · 2025-10-17T21:54:11Z

packages/shield-controller/src/backend.ts

+      this.#pollingWithTimeout
+        .pollRequest(config.requestId, requestCoverageFn, pollingOptions)
+        .then(resolve)
+        .catch(reject);


The refactor makes everything much more readable. 👍

Now you can even drop the wrapping new Promise and just write return await this.#pollingWithTimeout.pollRequest(...);.

matthiasgeihs · 2025-10-17T22:04:33Z

packages/shield-controller/src/polling-with-timeout-abort.ts

+          throw new Error(`${pollingOptions.fnName}: Request cancelled`);
+        }
+      }
+      await this.#delay(pollInterval);


Note that this is not canceled by the abort signal. So if we have a long polling interval, then it can take some time until this returns.
Previously this was achieved by rejecting immediately when the timeout occurred.

Yes, the abort doesn't cancelled the delay between polling.
But I don't think this is a big issue, a small delay won't hurt so much. After the delay, the polling will break.

matthiasgeihs · 2025-10-17T22:06:13Z

packages/shield-controller/src/polling-with-timeout-abort.ts

+      try {
+        const result = await requestFn(abortController.signal);
+        // polling success, we just need to clean up the request entry and return the result
+        this.#cleanUpOnFinished(requestId);


Do you have to call this cleanup function also in the catch case?

Not really, coz cleanUpOnFinished will be called in the AbortHandler and Timeout handle, where we only want to clean up.
We don't want to clean in case of normal errors, we want the polling to continue.

matthiasgeihs · 2025-10-17T22:16:20Z

packages/shield-controller/src/polling-with-timeout-abort.ts

+    this.abortPendingRequests(requestId);
+
+    // insert the request entry for the next polling cycle
+    const { abortController } = this.#insertRequestEntry(requestId, timeout);


The whole functionality of this is a bit difficult to grasp as it's spread across the whole file and different handlers (especially the different cleanup handlers are confusing).

Would it be possible to streamline the logic a bit:

Create 1 abort controller. If this aborts, the polling aborts and the abort handling is cleaned up.

Attached the timeout event to the abort controller. (i.e., on timeout, call controller.abort())

lwin-kyaw added 2 commits October 16, 2025 05:42

fix: added cancellation and proper timeout logic to coverage result p…

eb2666b

…olling.

test: added tests for coverage

0ce87d5

lwin-kyaw requested a review from a team as a code owner October 15, 2025 21:46

This comment was marked as outdated.

Sign in to view

fix: abort/cancel coverage result requests before logging.

7b18fd5

This comment was marked as outdated.

Sign in to view

feat: removed 'personalSign' check from signature coverage and some c…

2fe24ea

…lean up

This comment was marked as outdated.

Sign in to view

doc: updated changelog

a2a9f78

lwin-kyaw requested a review from a team as a code owner October 16, 2025 04:51

matthiasgeihs requested changes Oct 16, 2025

View reviewed changes

matthiasgeihs reviewed Oct 16, 2025

View reviewed changes

packages/shield-controller/src/backend.ts Outdated Show resolved Hide resolved

feat: created map of abort-controllers to track the multiple tx/sig r…

9225267

…equests

This comment was marked as outdated.

Sign in to view

lwin-kyaw requested a review from matthiasgeihs October 16, 2025 10:32

apply abort.signal to result request

9119e4a

matthiasgeihs reviewed Oct 17, 2025

View reviewed changes

lwin-kyaw added 2 commits October 18, 2025 00:39

chore: rename some methods/vars and removed unnecessary mocks from test

bfa117c

chore: refactor backend.test.ts

affbbcf

This comment was marked as outdated.

Sign in to view

refactor: refactor polling logic and clean up tests

565c42c

This comment was marked as outdated.

Sign in to view

feat: updated log methods with abortPendingRequests

a1d580c

This comment was marked as outdated.

Sign in to view

chore: added error reason, timeout or cancelled in polling

19e8a3e

This comment was marked as outdated.

Sign in to view

chore: update comments

588dd38

lwin-kyaw added 2 commits October 18, 2025 05:39

chore: resolved conflicts

7ed65c0

fix: fixed ChangeLog

a5295ab

matthiasgeihs reviewed Oct 17, 2025

View reviewed changes

	#cancelPreviousPendingRequests(id: string) {
	#cancelPendingRequests(id: string) {

Uh oh!

fix: shield coverage result polling #6847

Are you sure you want to change the base?

fix: shield coverage result polling #6847

Uh oh!

Conversation

lwin-kyaw commented Oct 15, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Checklist

Uh oh!

This comment was marked as outdated.

Uh oh!

lwin-kyaw commented Oct 15, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

lwin-kyaw commented Oct 16, 2025

Uh oh!

matthiasgeihs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwin-kyaw commented Oct 15, 2025 •

edited by cursor bot

Loading

matthiasgeihs Oct 17, 2025 •

edited

Loading